[67724] Make breadcrumbs optional in page header component - #376
Conversation
…ontext bar only if there are anything in it to be shown
🦋 Changeset detectedLatest commit: a8ddab6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
HDinger
left a comment
HDinger
left a comment
There was a problem hiding this comment.
Did you check locally in core whether the spacing matches so that the title of a PageHeader without breadcrumb is at the same height as the breadcrumb on other pages? I want to avoid special alignments for the menu button when there is no breadcrumb. Also, it would create visual jumps when navigating between pages.
| # @param items [Array<String, Hash>] Items is an array of strings, hash {href, text} or an anchor tag string | ||
| # @param system_arguments [Hash] <%= link_to_system_arguments_docs %> | ||
| renders_one :breadcrumbs, lambda { |items, selected_item_font_weight: :bold, **system_arguments| | ||
| renders_one :breadcrumbs, lambda { |items = nil, selected_item_font_weight: :bold, **system_arguments| |
There was a problem hiding this comment.
with the default provided, nothing forces other devs to actually think about the breadcrumb. You can call
render(Primer::OpenProject::PageHeader.new) do |header|
header.with_title { "Hello" }
end
and it would be valid. Since in most cases we want a breadcrumb, we have to enforce that stronger. Please remove the default, so that the caller has to explicitly set nil if he/she wants to make an exception.
There was a problem hiding this comment.
It is not valid, it returns an error:
PageHeader needs a title and a breadcrumb. Please use the with_title and with_breadcrumbs slot
There was a problem hiding this comment.
I'd nevertheless prefer it to be set explicitly. Having to use an empty slot which renders nothing in the end, feels weird.
So I'd prefer with_breadcrumbs(nil)
HDinger
left a comment
HDinger
left a comment
There was a problem hiding this comment.
Nice 👍 I only have small remarks left
| .PageHeader--noBreadcrumb .PageHeader-contextBar { | ||
| display: none; | ||
| } | ||
|
|
||
| /* Keep it visible on smaller */ | ||
| @media (max-width: 543.98px) { | ||
| .PageHeader--noBreadcrumb .PageHeader-contextBar { | ||
| display: flex; | ||
| } | ||
| } |
There was a problem hiding this comment.
| .PageHeader--noBreadcrumb .PageHeader-contextBar { | |
| display: none; | |
| } | |
| /* Keep it visible on smaller */ | |
| @media (max-width: 543.98px) { | |
| .PageHeader--noBreadcrumb .PageHeader-contextBar { | |
| display: flex; | |
| } | |
| } | |
| /* Hide the context bar on desktop when no breadcrumb is visible */ | |
| @media screen and (min-width: $breakpoint-sm) { | |
| .PageHeader--noBreadcrumb .PageHeader-contextBar { | |
| display: none; | |
| } | |
| } |
| } | ||
|
|
||
| .PageHeader--noBreadcrumb .PageHeader-titleBar { | ||
| height: var(--control-small-size); |
There was a problem hiding this comment.
This was probably done for the alignment with the menu button? Could you please add a comment for that? Otherwise, it is weird to understand later.
2 participants
What are you trying to accomplish?
Make the breadcrumbs in page header component optional
Screenshots
Closes https://community.openproject.org/wp/67724
Risk Assessment